application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheMapKeys.java (1)

3-7: Make utility holder non-instantiable and final.

Prevents accidental instantiation/extension and clarifies intent.

-public class CacheMapKeys {
+public final class CacheMapKeys {
     public static final String ACTIONS = "actionsCache";
     public static final String FUNCTIONS = "functionsCache";
     public static final String NAMESPACE_FUNCTIONS = "namespaceFunctionsCache";
+
+    private CacheMapKeys() {
+        throw new AssertionError("No instances");
+    }
 }

application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)

22-24: Leaking the mutable Map via getNativeCache couples callers to internals.

FieldActionsCacheService manipulating the map directly bypasses cache invariants/locking.

Prefer cache APIs (get/put/evict/clear). If direct access is required, document it and ensure all such usage is confined and synchronized.

application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)

43-65: Confirm thread-safety of MaxSizeHashMap or wrap it.

If MaxSizeHashMap is not concurrent, concurrent access through the cache will race.

Options:

• Wrap: () -> java.util.Collections.synchronizedMap(new MaxSizeHashMap<>(...))
• Or rely on the synchronized GenericMapCache changes above.
• Add a brief Javadoc on MaxSizeHashMap usage here clarifying concurrency expectations.

application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (3)

142-143: Avoid raw cast in getNamespaceFunctionCache; ensure type-safety and null-safety.

Keeps method contract explicit and prevents CCE if the impl changes.

-        return new HashMap<>((Map) cacheManager.getCache(CacheMapKeys.NAMESPACE_FUNCTIONS).getNativeCache());
+        Map<String, List<CachedFunction>> nativeMap =
+                (Map<String, List<CachedFunction>>) getRequiredCache(CacheMapKeys.NAMESPACE_FUNCTIONS).getNativeCache();
+        return new HashMap<>(nativeMap);

---

147-148: Clear methods: use guarded cache retrieval.

-        cacheManager.getCache(CacheMapKeys.ACTIONS).clear();
+        getRequiredCache(CacheMapKeys.ACTIONS).clear();
@@
-        cacheManager.getCache(CacheMapKeys.NAMESPACE_FUNCTIONS).clear();
+        getRequiredCache(CacheMapKeys.NAMESPACE_FUNCTIONS).clear();
@@
-        cacheManager.getCache(CacheMapKeys.FUNCTIONS).clear();
+        getRequiredCache(CacheMapKeys.FUNCTIONS).clear();

Also applies to: 151-153, 156-158

---

74-84: Keep this service cache-implementation-agnostic; avoid getNativeCache() and instanceof Map.

Relying on the native Map couples you to GenericMapCache and its thread-safety characteristics. Using Cache APIs (get(key, Callable), evict, clear) preserves portability and simplifies reasoning.

Also applies to: 89-99, 142-143

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)

15-15: Make map-backed cache thread-safe; compute-and-store must be atomic.

Backed by a plain Map with unsynchronized mutations; races with put/evict/clear and get(loader). Clear also reassigns the map mid-flight. Synchronize mutations and use double-checked locking in get(loader).

 public abstract class GenericMapCache<V> implements Cache {
@@
-    protected volatile Map<String, V> map;
+    protected volatile Map<String, V> map;
+    private final Object monitor = new Object();
@@
-    @Override
-    public <T> T get(Object key, java.util.concurrent.Callable<T> loader) {
+    @Override
+    public <T> T get(Object key, java.util.concurrent.Callable<T> loader) {
         String stringKey = String.valueOf(key);
-        Object present = map.get(stringKey);
+        Object present = map.get(stringKey);
 
         if (present != null) {
             return (T) present;
         }
 
-        try {
-            T computed = loader.call();
-            if (computed != null) {
-                map.put(stringKey, safeCast(computed));
-            }
-            return computed;
-        } catch (Exception ex) {
-            throw new org.springframework.cache.Cache.ValueRetrievalException(stringKey, loader, ex);
-        }
+        synchronized (monitor) {
+            present = map.get(stringKey);
+            if (present != null) {
+                return (T) present;
+            }
+            try {
+                T computed = loader.call();
+                if (computed != null) {
+                    map.put(stringKey, safeCast(computed));
+                }
+                return computed;
+            } catch (Exception ex) {
+                throw new org.springframework.cache.Cache.ValueRetrievalException(stringKey, loader, ex);
+            }
+        }
     }
@@
-    public void put(Object key, Object value) {
-        map.put(String.valueOf(key), safeCast(value));
+    public void put(Object key, Object value) {
+        synchronized (monitor) {
+            map.put(String.valueOf(key), safeCast(value));
+        }
     }
@@
-    public void evict(Object key) {
-        map.remove(String.valueOf(key));
+    public void evict(Object key) {
+        synchronized (monitor) {
+            map.remove(String.valueOf(key));
+        }
     }
@@
-    public void clear() {
-        this.map = mapFactory.get();
+    public void clear() {
+        synchronized (monitor) {
+            this.map = mapFactory.get();
+        }
         log.info("{} cache cleared", this.getName());
     }

Also applies to: 33-51, 53-67

application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (4)

55-62: Fail fast on missing cache; avoid NPE.

-        Cache namespaceFunctionsCache = cacheManager.getCache(CacheMapKeys.NAMESPACE_FUNCTIONS);
+        Cache namespaceFunctionsCache = getRequiredCache(CacheMapKeys.NAMESPACE_FUNCTIONS);
@@
-            namespaceFunctionsCache.evictIfPresent(petriNet.getIdentifier());
+            namespaceFunctionsCache.evictIfPresent(petriNet.getIdentifier());

Support code (add once in this class):

private Cache getRequiredCache(String name) {
    Cache cache = cacheManager.getCache(name);
    if (cache == null) throw new IllegalStateException("Cache '" + name + "' not configured");
    return cache;
}

---

65-69: Guard cache retrieval and reload atomically.

-    public void reloadCachedFunctions(String petriNetId) {
-        cacheManager.getCache(CacheMapKeys.NAMESPACE_FUNCTIONS).evictIfPresent(petriNetId);
-        cachePetriNetFunctions(petriNetService.getNewestVersionByIdentifier(petriNetId));
-    }
+    public void reloadCachedFunctions(String petriNetId) {
+        getRequiredCache(CacheMapKeys.NAMESPACE_FUNCTIONS).evictIfPresent(petriNetId);
+        cachePetriNetFunctions(petriNetService.getNewestVersionByIdentifier(petriNetId));
+    }

---

79-89: Use Cache APIs; avoid peeking into native Map and eliminate TOCTOU.

-        Cache actionsCache = cacheManager.getCache(CacheMapKeys.ACTIONS);
-        Object nativeActionsCache = actionsCache.getNativeCache();
-
-        if (nativeActionsCache instanceof Map<?, ?> map) {
-            if (shouldRewriteCachedActions || !map.containsKey(stringId) ) {
-                Closure code = (Closure) shell.evaluate("{-> " + action.getDefinition() + "}");
-                actionsCache.put(stringId, code);
-            }
-        }
-        return (Closure) actionsCache.get(stringId).get();
+        Cache actionsCache = getRequiredCache(CacheMapKeys.ACTIONS);
+        if (shouldRewriteCachedActions) {
+            actionsCache.evict(stringId);
+        }
+        return actionsCache.get(stringId,
+                () -> (Closure) shell.evaluate("{-> " + action.getDefinition() + "}"));

---

93-105: Compute functions atomically and avoid native cache access.

-        Cache functionsCache = cacheManager.getCache(CacheMapKeys.FUNCTIONS);
-        Object nativeFunctionsCache = functionsCache.getNativeCache();
-
-        if (nativeFunctionsCache instanceof Map<?, ?> map) {
-            functions.forEach(function -> {
-                if (!map.containsKey(function.getStringId())) {
-                    functionsCache.put(function.getStringId(), CachedFunction.build(shell, function));
-                }
-                cachedFunctions.add((CachedFunction) functionsCache.get(function.getStringId()).get());
-            });
-        }
+        Cache functionsCache = getRequiredCache(CacheMapKeys.FUNCTIONS);
+        for (Function function : functions) {
+            CachedFunction cf = functionsCache.get(function.getStringId(),
+                    () -> CachedFunction.build(shell, function));
+            cachedFunctions.add(cf);
+        }

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: tuplle
PR: netgrif/application-engine#334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.

application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsNamespaceMapCache.java (2)

21-32: Honor Cache.get contract; avoid NPE; remove unnecessary PetriNet fetch; handle contention.

• Return null on miss, not ValueWrapper(null).
• Use reload-by-id API; fetching PetriNet is unnecessary.
• Prevent thundering herd by delegating to GenericMapCache.get(key, loader).

Apply:

     @Override
-    public ValueWrapper get(Object key) {
-        String stringKey = String.valueOf(key);
-
-        Object valueObject = map().get(stringKey);
-        if (valueObject != null) {
-            return new SimpleValueWrapper(List.copyOf((List<CachedFunction>) valueObject));
-        }
-        PetriNet petriNet = petriNetService.getPetriNet(stringKey);
-        fieldActionsCacheService.cachePetriNetFunctions(petriNet);
-        return new SimpleValueWrapper(List.copyOf((List<CachedFunction>) map().get(stringKey)));
-    }
+    public ValueWrapper get(Object key) {
+        if (key == null) throw new IllegalArgumentException("Cache key must not be null");
+        final String stringKey = key.toString();
+        List<CachedFunction> value = this.get(stringKey, () -> {
+            fieldActionsCacheService.reloadCachedFunctions(stringKey);
+            List<CachedFunction> v = (List<CachedFunction>) map().get(stringKey);
+            return v == null ? null : List.copyOf(v);
+        });
+        return value == null ? null : new SimpleValueWrapper(value);
+    }

---

34-46: Override typed get; mirror null-miss semantics; avoid PetriNet fetch.

Also add key validation and contention-safe loader.

Apply:

-    public <T> T get(Object key, Class<T> type) {
-        String stringKey = String.valueOf(key);
-        Object valueObject = map().get(stringKey);
-
-        if (valueObject != null) {
-            return type.cast(List.copyOf((List<CachedFunction>) valueObject));
-        }
-
-        PetriNet petriNet = petriNetService.getPetriNet(stringKey);
-        fieldActionsCacheService.cachePetriNetFunctions(petriNet);
-        return type.cast(List.copyOf((List<CachedFunction>) map().get(stringKey)));
-
-    }
+    @Override
+    public <T> T get(Object key, Class<T> type) {
+        if (key == null) throw new IllegalArgumentException("Cache key must not be null");
+        final String stringKey = key.toString();
+        List<CachedFunction> value = this.get(stringKey, () -> {
+            fieldActionsCacheService.reloadCachedFunctions(stringKey);
+            List<CachedFunction> v = (List<CachedFunction>) map().get(stringKey);
+            return v == null ? null : List.copyOf(v);
+        });
+        return value == null ? null : type.cast(value);
+    }

application-engine/src/main/java/com/netgrif/application/engine/configuration/FunctionsMapCache.java (3)

15-22: Parameterize the cache; align with stored type; add @OverRide imports not needed.

Removes raw type usage and keeps type-safety.

Apply:

-@Slf4j
-public class FunctionsMapCache extends GenericMapCache {
+@Slf4j
+public class FunctionsMapCache extends GenericMapCache<CachedFunction> {
@@
-    public FunctionsMapCache(String name, java.util.function.Supplier<Map<String, CachedFunction>> mapFactory, IFieldActionsCacheService fieldActionsCacheService, IPetriNetService petriNetService, IGroovyShellFactory shellFactory) {
-        super(name, CachedFunction.class, mapFactory, fieldActionsCacheService, petriNetService);
+    public FunctionsMapCache(String name,
+                             java.util.function.Supplier<Map<String, CachedFunction>> mapFactory,
+                             IFieldActionsCacheService fieldActionsCacheService,
+                             IPetriNetService petriNetService,
+                             IGroovyShellFactory shellFactory) {
+        super(name, CachedFunction.class, mapFactory, fieldActionsCacheService, petriNetService);
         this.shell = shellFactory.getGroovyShell();
     }

---

25-40: Fix miss semantics; avoid double build; use super.put; validate key.

• Return null when absent.
• Build once on miss; reuse built instance for storage and response (or consistently return a copy).

Apply:

     @Override
     public synchronized ValueWrapper get(Object key) {
-        String stringKey = String.valueOf(key);
+        if (key == null) throw new IllegalArgumentException("Cache key must not be null");
+        String stringKey = key.toString();
 
         Object valueObject = map().get(stringKey);
         if (valueObject != null) {
-            return new SimpleValueWrapper(CachedFunction.copyOf(shell, (CachedFunction) valueObject));
+            return new SimpleValueWrapper(CachedFunction.copyOf(shell, (CachedFunction) valueObject));
         }
 
         Function function = petriNetService.findByFunctionId(stringKey);
         if (function != null) {
-            map().put(stringKey, CachedFunction.build(shell, function));
-            return new SimpleValueWrapper(CachedFunction.build(shell, function));
+            CachedFunction built = CachedFunction.build(shell, function);
+            super.put(stringKey, built);
+            return new SimpleValueWrapper(CachedFunction.copyOf(shell, built));
         }
 
-        return new SimpleValueWrapper(null);
+        return null;
     }

---

42-57: Override typed get; unify semantics with untyped; return null on miss.

Also avoid double build and write via super.put.

Apply:

-    public synchronized  <T> T get(Object key, Class<T> type) {
-        String stringKey = String.valueOf(key);
+    @Override
+    public synchronized <T> T get(Object key, Class<T> type) {
+        if (key == null) throw new IllegalArgumentException("Cache key must not be null");
+        String stringKey = key.toString();
         Object valueObject = map().get(stringKey);
 
         if (valueObject != null) {
-            return type.cast(valueObject);
+            return type.cast(CachedFunction.copyOf(shell, (CachedFunction) valueObject));
         }
 
         Function function = petriNetService.findByFunctionId(stringKey);
         if (function != null) {
-            map().put(stringKey, CachedFunction.build(shell, function));
-            return type.cast(CachedFunction.build(shell, function));
+            CachedFunction built = CachedFunction.build(shell, function);
+            super.put(stringKey, built);
+            return type.cast(CachedFunction.copyOf(shell, built));
         }
 
-        return type.cast(null);
+        return null;
     }

application-engine/src/main/java/com/netgrif/application/engine/configuration/ActionsMapCache.java (3)

12-12: Parameterize the cache with Closure to eliminate raw types

Extend GenericMapCache<Closure> for compile-time safety.

- public class ActionsMapCache extends GenericMapCache {
+ public class ActionsMapCache extends GenericMapCache<Closure> {

---

22-28: Honor Cache contract: return null on miss, don’t wrap null

Wrapping null in SimpleValueWrapper violates Spring’s Cache contract.

         Object valueObject = map().get(stringKey);
         if (valueObject != null) {
             return new SimpleValueWrapper(valueObject);
         }
         fieldActionsCacheService.reloadCachedFunctions(stringKey);
-        return new SimpleValueWrapper(map().get(stringKey));
+        Object refreshed = map().get(stringKey);
+        return refreshed != null ? new SimpleValueWrapper(refreshed) : null;

---

30-41: Add @OverRide and mirror null semantics for typed get

Also avoid type.cast(null) and return null directly.

-    public synchronized <T> T get(Object key, Class<T> type) {
+    @Override
+    public synchronized <T> T get(Object key, Class<T> type) {
         String stringKey = String.valueOf(key);
         Object valueObject = map().get(stringKey);
 
         if (valueObject != null) {
             return type.cast(valueObject);
         }
 
         fieldActionsCacheService.reloadCachedFunctions(stringKey);
-        return type.cast(map().get(stringKey));
+        Object refreshed = map().get(stringKey);
+        return refreshed != null ? type.cast(refreshed) : null;
 
     }

Run to locate similar violations across caches:

#!/bin/bash
rg -nP --type=java 'SimpleValueWrapper\\(null\\)|return\\s+type\\.cast\\(null\\)'

application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)

17-18: Tighten generics: use Class and make it final

Reduces unchecked casts and encodes the value type at the compiler level.

-    protected Class<?> valueType;
+    protected final Class<V> valueType;
@@
-    public GenericMapCache(String name, Class<?> valueType, java.util.function.Supplier<Map<String, V>> mapFactory, IFieldActionsCacheService fieldActionsCacheService, IPetriNetService petriNetService) {
+    public GenericMapCache(String name, Class<V> valueType, java.util.function.Supplier<Map<String, V>> mapFactory, IFieldActionsCacheService fieldActionsCacheService, IPetriNetService petriNetService) {

Also applies to: 26-26

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

application-engine/src/main/java/com/netgrif/application/engine/configuration/GenericMapCache.java (1)

59-61: Fix compile error: qualify ValueRetrievalException

It’s an inner type of Cache.

-        } catch (Exception ex) {
-            throw new ValueRetrievalException(stringKey, loader, ex);
+        } catch (Exception ex) {
+            throw new Cache.ValueRetrievalException(stringKey, loader, ex);
         }

application-engine/src/main/java/com/netgrif/application/engine/workflow/service/FieldActionsCacheService.java (6)

58-65: Fail fast on missing namespace functions cache

Use a required-cache helper to avoid NPEs and produce clear errors.

-        Cache namespaceFunctionsCache = cacheManager.getCache(properties.getNamespaceFunctions());
+        Cache namespaceFunctionsCache = getRequiredCache(properties.getNamespaceFunctions());

---

149-151: Return a typed copy of the namespace cache; avoid raw casts

Safer and future‑proof.

-        return new HashMap<>((Map) cacheManager.getCache(properties.getNamespaceFunctions()).getNativeCache());
+        @SuppressWarnings("unchecked")
+        Map<String, List<CachedFunction>> nativeMap =
+                (Map<String, List<CachedFunction>>) getRequiredCache(properties.getNamespaceFunctions()).getNativeCache();
+        return new HashMap<>(nativeMap);

---

154-166: Clear caches via required-cache helper

Prevents NPEs if a cache is missing.

-        cacheManager.getCache(CacheMapKeys.ACTIONS).clear();
+        getRequiredCache(CacheMapKeys.ACTIONS).clear();
@@
-        cacheManager.getCache(properties.getNamespaceFunctions()).clear();
+        getRequiredCache(properties.getNamespaceFunctions()).clear();
@@
-        cacheManager.getCache(CacheMapKeys.FUNCTIONS).clear();
+        getRequiredCache(CacheMapKeys.FUNCTIONS).clear();

---

69-72: Guard reload path with required-cache helper

Prevents NPE when cache is misconfigured.

-        cacheManager.getCache(properties.getNamespaceFunctions()).evictIfPresent(petriNetId);
+        getRequiredCache(properties.getNamespaceFunctions()).evictIfPresent(petriNetId);
         cachePetriNetFunctions(petriNetService.getNewestVersionByIdentifier(petriNetId));

---

80-92: Use atomic Cache#get(key, loader) and evict on rewrite; stop peeking into native map

This fixes potential NPE and races, and honors rewrite semantics.

-        Cache actionsCache = cacheManager.getCache(CacheMapKeys.ACTIONS);
-        Object nativeActionsCache = actionsCache.getNativeCache();
-
-        if (nativeActionsCache instanceof Map<?, ?> map) {
-            if (shouldRewriteCachedActions || !map.containsKey(stringId) ) {
-                Closure code = (Closure) shell.evaluate("{-> " + action.getDefinition() + "}");
-                actionsCache.put(stringId, code);
-            }
-        }
-        return (Closure) actionsCache.get(stringId).get();
+        Cache actionsCache = getRequiredCache(CacheMapKeys.ACTIONS);
+        if (shouldRewriteCachedActions) {
+            actionsCache.evict(stringId);
+        }
+        return actionsCache.get(stringId, () -> (Closure) shell.evaluate("{-> " + action.getDefinition() + "}"));

---

95-107: Compute functions via Cache API, no native map casting

Cleaner, thread-safe, and backend-agnostic.

-        Cache functionsCache = cacheManager.getCache(CacheMapKeys.FUNCTIONS);
-        Object nativeFunctionsCache = functionsCache.getNativeCache();
-
-        if (nativeFunctionsCache instanceof Map<?, ?> map) {
-            functions.forEach(function -> {
-                if (!map.containsKey(function.getStringId())) {
-                    functionsCache.put(function.getStringId(), CachedFunction.build(shell, function));
-                }
-                cachedFunctions.add((CachedFunction) functionsCache.get(function.getStringId()).get());
-            });
-        }
+        Cache functionsCache = getRequiredCache(CacheMapKeys.FUNCTIONS);
+        for (Function function : functions) {
+            CachedFunction cf = functionsCache.get(function.getStringId(),
+                    () -> CachedFunction.build(shell, function));
+            cachedFunctions.add(cf);
+        }

application-engine/src/main/java/com/netgrif/application/engine/configuration/CacheConfiguration.java (1)

38-42: Avoid duplicate registrations for specialized caches

Filter out ACTIONS and FUNCTIONS from the generic cache list before adding specialized implementations.

-        Set<String> cacheNames = properties.getAllCaches();
-        List<Cache> caches = cacheNames.stream()
+        Set<String> cacheNames = new LinkedHashSet<>(properties.getAllCaches());
+        cacheNames.removeAll(Set.of(CacheMapKeys.ACTIONS, CacheMapKeys.FUNCTIONS));
+        List<Cache> caches = cacheNames.stream()
                 .map(ConcurrentMapCache::new)
                 .collect(Collectors.toCollection(ArrayList::new));

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Learnt from: tuplle
PR: netgrif/application-engine#334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.